Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor MTE-3854 Integrate menu refactor work and xcode 16 #23381

Merged
merged 17 commits into from
Dec 2, 2024
Merged

Refactor MTE-3854 Integrate menu refactor work and xcode 16 #23381

merged 17 commits into from
Dec 2, 2024

Conversation

clarmso
Copy link
Collaborator

@clarmso clarmso commented Nov 25, 2024 •
edited
Loading

📜 Tickets

Jira ticket

💡 Description

Cherry pick menu refactor work after Xcode 16 is in use.

TODO

  • Changes required for full functional tests

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@clarmso clarmso requested review from a team as code owners November 25, 2024 20:08
@clarmso clarmso requested a review from isabelrios November 25, 2024 20:08
@clarmso clarmso mentioned this pull request Nov 25, 2024
Closed
6 tasks
@clarmso clarmso requested a review from mdotb-moz November 25, 2024 20:24
@mobiletest-ci-bot
Copy link

mobiletest-ci-bot commented Nov 25, 2024 •
edited
Loading

Messages
📖 Edited 20 files
📖 Created 0 files

Generated by 🚫 Danger Swift against 18b1dd7

clarmso
clarmso commented Nov 25, 2024
View reviewed changes
firefox-ios/firefox-ios-tests/Tests/UnitTest.xctestplan
@@ -119,6 +119,7 @@
"ShortcutRouteTests\/testOpenLastBookmarkShortcutWithInvalidUrl()",
"TabManagerTests\/testDeleteSelectedTab()",
"TabManagerTests\/testPrivatePreference_togglePBMDeletesPrivate()",
"TabToolbarHelperTests\/testTelemetryForSiteMenu()",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails with the menu refactor turned on. 😞

@clarmso
Copy link
Collaborator Author

clarmso commented Nov 26, 2024

@isabelrios Should we merge this PR just with the changes for smoke tests? 🙂 Please approve this PR if it's the case. We can address the full functional tests in a subsequent PR.
(cc @thatswinnie)

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 26, 2024

This pull request has conflicts when rebasing. Could you fix it @clarmso? 🙏

@mdotb-moz mdotb-moz force-pushed the cs/MTE-3854-menu-refactor-xcode-16 branch from b2c6b6a to 373343b Compare November 27, 2024 09:39
@mdotb-moz mdotb-moz force-pushed the cs/MTE-3854-menu-refactor-xcode-16 branch from 334372c to f8c6e01 Compare November 28, 2024 12:27
isabelrios
isabelrios reviewed Nov 28, 2024
View reviewed changes
firefox-ios/firefox-ios-tests/Tests/XCUITests/ToolbarMenuTests.swift Outdated
XCUIDevice.shared.orientation = .landscapeLeft
mozWaitForElementToExist(hamburgerMenu)
mozWaitForElementToNotExist(app.tables["Context Menu"])
// mozWaitForElementToNotExist(app.tables["Context Menu"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a comment why we comment these lines?

isabelrios
isabelrios reviewed Nov 28, 2024
View reviewed changes
firefox-ios/firefox-ios-tests/Tests/XCUITests/ToolbarMenuTests.swift Outdated
@@ -58,25 +61,25 @@ class ToolbarMenuTests: BaseTestCase {
"Menu button is not below the pocket cells area"
)
hamburgerMenu.tap()
mozWaitForElementToExist(app.tables["Context Menu"])
mozWaitForElementToExist(app.images[StandardImageIdentifiers.Large.avatarCircle])
// mozWaitForElementToExist(app.tables["Context Menu"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

clarmso
clarmso commented Nov 28, 2024
View reviewed changes
firefox-ios/firefox-ios-tests/Tests/XCUITests/TrackingProtectionTests.swift Outdated
[
app.staticTexts["mozilla.org"],
app.staticTexts["Secure connection"]
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not change the indentation (unless it's swiftlint related).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this indentation. It's not very Swifty. lolol oh well

@mdotb-moz mdotb-moz force-pushed the cs/MTE-3854-menu-refactor-xcode-16 branch from 40c8166 to fad7cd7 Compare December 2, 2024 11:30
@mdotb-moz mdotb-moz requested a review from isabelrios December 2, 2024 15:00
isabelrios
isabelrios approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@isabelrios isabelrios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address comment about swiftlint here or in a follow up

@clarmso
Copy link
Collaborator Author

clarmso commented Dec 2, 2024

Branch with menu refactor off before merging this PR: https://github.com/mozilla-mobile/firefox-ios/tree/release/NoMenuRefactorDec2

mdotb-moz
mdotb-moz approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@mdotb-moz mdotb-moz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@clarmso clarmso merged commit c8680a6 into main Dec 2, 2024
10 checks passed
@clarmso clarmso deleted the cs/MTE-3854-menu-refactor-xcode-16 branch December 2, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adudenamedruby adudenamedruby adudenamedruby left review comments

@isabelrios isabelrios isabelrios approved these changes

@mdotb-moz mdotb-moz mdotb-moz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@clarmso @mobiletest-ci-bot @isabelrios @adudenamedruby @mdotb-moz